fix(security): remove shell injection vector in Windows process spawning#2687
fix(security): remove shell injection vector in Windows process spawning#2687Ivorisnoob wants to merge 7 commits into
Conversation
ssh, tailscale, git, and powershell.exe are native executables and do not need cmd.exe as an intermediary. Passing shell:true caused Node.js to route args through `cmd.exe /d /s /c "..."`, where metacharacters like & and | are interpreted as command separators. A user-controlled hostname in the SSH flow could trigger arbitrary command execution. npm-installed CLIs (claude, codex, cursor) are intentionally left with shell:true as they may only exist as .cmd wrappers on Windows.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Needs human review Changes to SSH process spawning code in a security-sensitive package. While removing shell execution hardens against injection, the original Windows-specific behavior existed for functional reasons and this change could affect Windows runtime behavior. Security-sensitive code paths warrant human review. You can customize Macroscope's approvability policy. Learn more. |
|
problem is most of these just doesn't work without |
Yeah you're right about the others, reverted those. Kept only the SSH one because ssh.exe is a native exe on Windows 11 (confirmed at C:\Windows\System32\OpenSSH\ssh.exe), so it doesn't need cmd.exe at all. And the hostname is literally the only spot in the whole codebase where user-typed input goes straight into spawn args without any sanitization, so that one's a real issue. |
What Changed
Replaced
shell: process.platform === "win32"withshell: falseforseven spawn sites that invoke native Windows executables:
packages/ssh/src/command.ts—sshpackages/ssh/src/tunnel.ts—sshpackages/tailscale/src/tailscale.ts(×2) —tailscaleapps/server/src/project/Layers/RepositoryIdentityResolver.ts(×2) —gitapps/server/src/diagnostics/ProcessDiagnostics.ts—powershell.exe/psapps/server/src/terminal/Layers/Manager.ts—powershell.exenpm-installed CLIs (
claude,codex,cursor,opencode) are leftunchanged — on Windows these may only exist as
.cmdbatch wrappers,which require shell resolution to execute.
Why
With
shell: true, Node.js constructs:cmd.exeinterprets&,|, and;as command separators eveninside this string. The SSH hostname is user-controlled and passed
through with no sanitization, so a value like:
causes
cmd.exeto download and execute the payload immediately afterthe (failing) SSH attempt — silent RCE on the developer's machine.
ssh.exe,tailscale.exe,git.exe, andpowershell.exeare allnative executables available on
%PATH%. They spawn correctly withshell: false, which passes the args array directly to the OS with noshell interpretation.
UI Changes
N/A
Checklist
Note
Medium Risk
Changes how
sshis spawned (nocmd.exeshell) which is security-positive but could affect Windows execution in environments wheresshisn’t a resolvable native executable on PATH.Overview
Disables shell-based spawning for SSH on all platforms. Both
runSshCommandInScope(command.ts) andstartSshTunnel(tunnel.ts) now setshell: falsewhen spawningssh, instead of conditionally enabling a shell on Windows.This reduces the Windows command-injection surface by ensuring the argument array is passed directly to the OS rather than being interpreted by
cmd.exe.Reviewed by Cursor Bugbot for commit d4a9d9b. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Remove shell injection vector in Windows SSH process spawning
Sets
shell: falseunconditionally in bothrunSshCommandInScopeandstartSshTunnelin command.ts and tunnel.ts, replacing the previousprocess.platform === "win32"conditional. This eliminates a shell injection risk that existed when spawning SSH child processes on Windows.Macroscope summarized d4a9d9b.